fix(skills): inject Skill into --tools when explicit tools= is set (#977)#985
Open
Zeffut wants to merge 1 commit into
Open
fix(skills): inject Skill into --tools when explicit tools= is set (#977)#985Zeffut wants to merge 1 commit into
Zeffut wants to merge 1 commit into
Conversation
…nthropics#977) When users pass an explicit tools=[...] list alongside skills="all", the CLI's --tools argument did not include Skill, so the Skill tool was authorized but never loaded. Centralizes the injection inside _apply_skills_defaults via an effective_tools return value used by _build_command. Idempotent, no-op when skills is unset. Adds 5 regression tests. mypy + ruff clean. 744 passed. Alternative approach to anthropics#979. Closes anthropics#977. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a regression fix for skills + explicit tools handling in SubprocessCLITransport so that selecting skills ensures the base --tools includes Skill, and extends the test suite to prevent regressions.
Changes:
- Update skills defaulting logic to optionally inject
Skillinto an explicit list-formtoolsoption. - Adjust
_build_command()to use the skills-adjustedtoolslist when building the--toolsflag. - Add regression tests covering injection behavior, idempotency, and immutability.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_transport.py | Adds regression tests ensuring Skill is added to --tools when skills are enabled with explicit tools, without mutating options. |
| src/claude_agent_sdk/_internal/transport/subprocess_cli.py | Extends _apply_skills_defaults() to also compute effective tools, and wires it into _build_command(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+227
to
238
| if tools is not None and "Skill" not in tools: | ||
| tools.append("Skill") | ||
| else: | ||
| for name in skills: | ||
| pattern = f"Skill({name})" | ||
| if pattern not in allowed_tools: | ||
| allowed_tools.append(pattern) | ||
| # The CLI's --tools flag matches the bare tool name; allowed_tools | ||
| # supplies the per-skill filtering. Ensure Skill is loadable. | ||
| if tools is not None and "Skill" not in tools: | ||
| tools.append("Skill") | ||
|
|
Comment on lines
+641
to
+642
| assert "--tools" in cmd | ||
| assert cmd[cmd.index("--tools") + 1] == "WebSearch,WebFetch,Read,Skill" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #977.
When users pass an explicit
tools=[...]list alongsideskills="all"(orskills=["pdf", ...]), the CLI receives--tools <user_list>and--allowedTools Skill. The CLI loads only the tools listed in--tools, soSkillis authorized but never loaded — the model cannot invoke any skill. This contradicts the documented behavior ofallowed_toolsas the single configuration knob for skills.Root cause
SubprocessCLITransport._apply_skills_defaultsinsrc/claude_agent_sdk/_internal/transport/subprocess_cli.pyinjectsSkillintoallowed_toolsbut never touches the basetoolslist._build_commandemits--toolsfromself._options.toolsdirectly. The two paths never meet.Fix
_apply_skills_defaultsnow returns a third value,effective_tools, which mirrors the caller'stoolslist withSkillappended whenskillsis set. Idempotent; only whentoolsis an explicit list — preset objects andNoneare left untouched. The original options object is not mutated._build_commandemits--toolsfrom this effective list.Behavior matrix
toolsskills--toolsemittedNonedefault(unchanged)[]""(unchanged)["Read"]NoneRead(unchanged)["Read"]"all"Read,Skill(FIXED)["Read"]["pdf"]Read,Skill(FIXED;allowed_toolsisSkill(pdf))["Read","Skill"]"all"Read,Skill(idempotent)Files modified
src/claude_agent_sdk/_internal/transport/subprocess_cli.py—_apply_skills_defaultsnow also returns effectivetools;_build_commanduses it.tests/test_transport.py— 5 new regression tests covering injection, idempotency, no-skills passthrough, and non-mutation.Verification
Python 3.12 venv,
pip install -e ".[dev]":python -m pytest tests/ --ignore=tests/test_build_wheel.py-> 744 passed, 5 skipped (5 new tests; baseline 739 passed).python -m mypy src/-> Success: no issues found in 24 source files.python -m ruff check src/ tests/-> All checks passed!Relation to #979
This is an alternative approach to #979, which is also open and targets the same bug. #979 injects in
_build_commanditself; this PR centralizes all skills-related logic in_apply_skills_defaults(the same helper that already handlesallowed_toolsandsetting_sources), which is easier to reason about and test in isolation. Happy to defer to whichever approach maintainers prefer.Test plan
pytest tests/ --ignore=tests/test_build_wheel.pygreen (744 passed)mypy src/cleanruff check src/ tests/cleantests/test_transport.py